Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove rustfmt from progenitor-impl #368

Merged
merged 1 commit into from
Apr 25, 2023
Merged

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Mar 18, 2023

Closes #362

progenitor-impl/src/lib.rs Outdated Show resolved Hide resolved
@jayvdb jayvdb marked this pull request as ready for review March 18, 2023 12:39
@jayvdb
Copy link
Contributor Author

jayvdb commented Mar 18, 2023

If this is what you had in mind, I'll update the README etc.

Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good direction; sorry it took me so long to get to this.

progenitor-impl/src/cli.rs Outdated Show resolved Hide resolved
@@ -505,41 +505,19 @@ impl Generator {

/// Render text output.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just delete generate_text and generatex_text_normalize_comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generate_text has been in the README.md sample build.rs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still! let's get rid of them!

@@ -26,6 +26,16 @@ where
}
}

fn generate_formatted(generator: &mut Generator, spec: &OpenAPI) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make the goal to have no changes in the output fixtures

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still one changed fixture - progenitor-impl/tests/output/nexus-cli.out - this is because the removed formatting code in cli.rs had different rustfmt settings, specifically it used only format_strings: Some(true), which wasnt used in lib.rs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's it look like if we format_strings: Some(true) for everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it looks great - it does however change the non-cli .out files, so was optional which approach to take. I'll add it and you can judge for yourself, but IMO it is worth the extra changes.

progenitor/src/main.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good; one outstanding item for you to consider; one conflict to resolve and I'll merge it

@jayvdb
Copy link
Contributor Author

jayvdb commented Apr 25, 2023

ready to roll @ahl

@ahl ahl merged commit b717086 into oxidecomputer:main Apr 25, 2023
@ahl
Copy link
Collaborator

ahl commented Apr 25, 2023

Thanks for doing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove implicit dependency for transitive consumers on rustfmt installation
2 participants